-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix container collect all assignment #811
base: main
Are you sure you want to change the base?
Conversation
@@ -129,7 +129,7 @@ func NewHelmInstallation(e config.CommonEnvironment, args HelmInstallationArgs, | |||
linuxInstallName += "-linux" | |||
} | |||
|
|||
values := buildLinuxHelmValues(installName, agentImagePath, agentImageTag, clusterAgentImagePath, clusterAgentImageTag, randomClusterAgentToken.Result, !args.DisableLogsContainerCollectAll) | |||
values := buildLinuxHelmValues(installName, agentImagePath, agentImageTag, clusterAgentImagePath, clusterAgentImageTag, randomClusterAgentToken.Result, args.DisableLogsContainerCollectAll) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is what we want? The buildLinuxHelmValues
has a parameter named logsContainerCollectAll
that will enable collection of all container logs when enabled. If you change that when DisableLogsContainer
is true we will enable logs collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too, but after experimenting with the initial configuration, I found that the previous method set container_collect_all
to true in the agent when WithoutLogsContainerCollectAll
was applied, whereas with this configuration container_collect_all
is correctly set to off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was because I missed this: #814
The disableContainerCollectAll
was never passed so no matter if you used WithoutLogsContainerCollectAll
the default value was passed to buildLinuxHelmValues
(ie !false
) and I think that's why it was always enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh okay gotcha, thanks for the clarification.
What does this PR do?
Fixes disabling
container_collect_all
in the agent running in Kubernetes.Which scenarios this will impact?
E2E tests in Kubernetes where the agent is orchestrated with
WithoutLogsContainerCollectAll()
.Motivation
Needed to be fixed for my E2E test to work.
Additional Notes